implement dclogin scheme#3541
Conversation
Simon-Laux
force-pushed
the
dclogin_scheme
branch
from
c5b0af8 to
3f19fdf
Compare
Simon-Laux
force-pushed
the
dclogin_scheme
branch
from
cafb2ca to
34390cd
Compare
Simon-Laux
requested review from
Hocuri,
adbenitez,
cyBerta,
flub,
link2xt,
missytake and
r10s
Hocuri
left a comment
Hocuri
left a comment
There was a problem hiding this comment.
I noticed that I don't know much (and don't have much of an opinion) about QR codes, so I ended up looking at the code style mainly. Which is nice, in the "big picture", since most of the new complexity could be put into one new file.
| Ok(()) | ||
| } | ||
|
|
||
| // idea: should invalid uri encoding result in error? |
There was a problem hiding this comment.
this is one of these todo points I was to lazy to tackle or did not immediately know how to or did not know if its worth the effort. anyways I don't really care, if you think such a test could be useful feel free to try making one 🤷
There was a problem hiding this comment.
so, either create a test or remove the comment. i do not think someone else will pick this task up by a random comment, that tends to be misunderstood months later.
Simon-Laux
force-pushed
the
dclogin_scheme
branch
from
01003f6 to
dfe5dab
Compare
|
Just so we're on the same page, my understanding is that this is waiting for @Simon-Laux to fix the CI and fix my remaining comments (or comment that/why he doesn't think this can/should be changed). Or @Simon-Laux would you like to hand this off to someone else? |
|
@r10s wanted to review it, but he also kept telling me how that isn't a priority for the 1.32 release, which should be out by now. anyhow I'm a bit confused here. Anyways I'm not against handing this over as long as the spec doesn't change and it still works afterwards.
|
There was a problem hiding this comment.
ci is also still unhappy :)
i made my points that i find the ::V1 bloated, we can add that later if really needed, esp. if we check for a version parameter (checking does not even require the parameter added to the qr code btw)
but i am also pretty sure that whatever addition we need in the future, we can go without ::V2 in the code. having a ::V2 and leaving ::V1 easily leads to legacy code lying around for rare cases, including possible bugs, potentially waiting for being misused. why already prepare this pattern without need?
but well, if everyone else is happy with ::V1, then so be it :) in general, the pr looks good to me, nice!
it is an documented interface to the outside, we don't control the creating of the qr codes, I personally think the forwards and backwards compatibility is worth it. With having a different version or variant, we can make certain parameters required and ensure the user scans it with a version of delta chat that supports it fully. V2 could also just be same as V1 just with videochat and a fileserver required for example, so the V1 would not even get obsolete. |
sure. i think, no one has misunderstood that. i just pointed out different, maybe easier and safer approaches - while still having the option for forwards and backwards compatibility - but i also pointed out that i do not see the current implementation as a blocker :) |
Co-authored-by: Hocuri <hocuri@gmx.de>
Simon-Laux
force-pushed
the
dclogin_scheme
branch
from
e2b94fa to
9615d98
Compare
to configure_from_login_qr
r10s
left a comment
r10s
left a comment
There was a problem hiding this comment.
rustfmt is unhappy and the comment "test idea" should be removed - or, better, a test added :)
sure, but I don't have a head for this anytime soon. Also it's kinda a theoretical problem anyway, In the sense that it is nice to have, but in the end it's up to the users to follow the spec. |
4 participants
see deltachat/interface#48 for the specification.
progress
deltachat.h
implementation
bindings